Skip to content

fix(modules): gate the module pin conflict on the cohort, not the flag - #3409

Closed
kwakayama wants to merge 3 commits into
mainfrom
fix/dependency-pinning-cohort-409
Closed

fix(modules): gate the module pin conflict on the cohort, not the flag#3409
kwakayama wants to merge 3 commits into
mainfrom
fix/dependency-pinning-cohort-409

Conversation

@kwakayama

@kwakayama kwakayama commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

Every client module of every hosted project answered 409 Unknown dependency snapshot in production. No page hydrated.

The defect

src/modules/server/module-server.ts:417 conflicted a module request whenever the pinning flag was set and the request carried no pin key:

(!hasRequestedPinKey && dependencyPinningEnabled)

That is the flag, not the cohort. Production runs VERYFRONT_DEPENDENCY_PINNING=1 with ROLLOUT_PERCENT=0 and no project allowlist, so no project is in the cohort and every document correctly emits no pin key. The document and the module handler therefore disagreed: one emitted no key, the other refused every request that lacked one.

dependency-pinning-cohort.ts states the property this broke:

"An absent or malformed percentage collapses to zero so a typo can never widen the rollout, which is the property that keeps the already-armed production pinning flag inert until it is deliberately ramped."

The armed flag was not inert. At 0% it conflicted 100% of client modules.

Reproduction

Locally, against the real production release, with the environment read off the live deployment rather than guessed:

env module request (no pins)
PINNING=1, ROLLOUT=0 (production's config) 409 Unknown dependency snapshot
PINNING=0 503 (unrelated local manifest branch)

Production returns the same 409 for /_vf_modules/pages/index.js, so the reproduction is faithful.

Fix

Consult the cohort, which is what decides who is actually pinning. A project genuinely in the cohort that omits its key still conflicts — that path is unchanged and its existing tests still pass.

Proof

Regression test written first, red before the change with the exact production symptom:

serves a client boundary for a project outside the pinning cohort ... FAILED
AssertionError: Values are not equal.
-   409
+   200

Green after. End to end with the rebuilt binary, production env, real release:

/_vf_modules/_veryfront/react/runtime/core.js   409  ->  200 (4964 B)

Zero 409s remain on any module path. Two project-module URLs return 503 in my sandbox from a separate release-asset-manifest branch that also fires with pinning disabled, so it is not this defect and not this fix.

Suite: 268 passed / 3344 steps across modules, security, handlers, and transforms/esm. deno fmt clean.

Not fixed here

The same pages also violate CSP: the document references esm.sh, images.veryfront.com, and fonts.googleapis.com while the default policy is 'self'-only. I confirmed this is independent of this fix — with modules serving correctly, the document still carries 6 esm.sh references and 291 image references.

I did try adding the ESM CDN to the default script-src and reverted it: an existing test, "default CSP admits no remote hosts or broad network schemes", fails with script-src must not hardcode a remote host. That strictness is deliberate, so widening it is a design decision rather than a bug fix, and it belongs in its own change with an owner who can make that call.

Summary by CodeRabbit

  • Bug Fixes
    • Corrected dependency-pinning rollout behavior for projects outside the configured rollout cohort.
    • Client-boundary modules now load successfully for out-of-cohort projects, even when dependency pinning is enabled.
  • Tests
    • Added regression coverage to verify successful JavaScript responses and proper rollout handling.

An armed VERYFRONT_DEPENDENCY_PINNING flag conflicted every client module of
every project while the rollout sat at 0%. The guard treated "no pin key" as
a conflict whenever the flag was set, but an out-of-cohort document correctly
emits no key, so the two disagreed and the browser got 409 Unknown dependency
snapshot for every module. Consulting the cohort restores the property the
rollout percent is supposed to have: the flag stays inert until ramped.
@kwakayama
kwakayama requested a review from kojiwakayama as a code owner August 6, 2026 05:57
@chatgpt-codex-connector

Copy link
Copy Markdown

Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits.
Repo admins can enable using credits for code reviews in their settings.

@coderabbitai

coderabbitai Bot commented Aug 6, 2026

Copy link
Copy Markdown

Review Change Stack

Warning

Review limit reached

@kwakayama, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 36 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 623c5192-45de-426c-9c69-31f255a689ff

📥 Commits

Reviewing files that changed from the base of the PR and between fc6ecef and d3b1791.

📒 Files selected for processing (2)
  • src/modules/server/module-server.test.ts
  • src/modules/server/module-server.ts
📝 Walkthrough

Walkthrough

serveModule now enforces missing-pin validation only for projects in the dependency-pinning rollout cohort. A regression test verifies successful unpinned client-module delivery for an out-of-cohort project.

Changes

Dependency-pinning rollout

Layer / File(s) Summary
Cohort-aware module gating
src/modules/server/module-server.ts
serveModule checks dependency-pinning cohort eligibility before enforcing a missing pin key. Invalid requested pin keys remain rejected.
Out-of-cohort regression coverage
src/modules/server/module-server.test.ts
The test sets rollout to 0%, verifies successful unpinned client-module delivery, and resets rollout and temporary state during cleanup.

Estimated code review effort: 2 (Simple) | ~10 minutes

Possibly related PRs

Suggested reviewers: kojiwakayama

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly identifies the main fix: applying module pin conflict handling by dependency-pinning cohort instead of the feature flag.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/dependency-pinning-cohort-409

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/modules/server/module-server.test.ts`:
- Around line 2880-2898: Update the test cleanup in the finally block for the
dependency-pinning test to restore the prior state of both
DEPENDENCY_PINNING_ENV_FLAG and DEPENDENCY_PINNING_ROLLOUT_PERCENT_ENV. If the
suite guarantees the flag was initially unset, clear DEPENDENCY_PINNING_ENV_FLAG
alongside the existing rollout cleanup.

In `@src/modules/server/module-server.ts`:
- Around line 418-424: Update the cohort check in serveModule’s
dependency-pinning condition to pass effectiveProjectId to
isProjectInDependencyPinningCohort instead of options.projectId, preserving the
existing flag and key checks. Add coverage for a request where projectId and
projectUUID differ, verifying cohort gating uses the effective identity and
returns the correct 409 behavior.
🪄 Autofix

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: f2a467cb-933e-4247-b9ae-437851af167f

📥 Commits

Reviewing files that changed from the base of the PR and between e92fa11 and fd4fe5f.

📒 Files selected for processing (2)
  • src/modules/server/module-server.test.ts
  • src/modules/server/module-server.ts

Comment thread src/modules/server/module-server.test.ts
Comment thread src/modules/server/module-server.ts Outdated
The suite already clears DEPENDENCY_PINNING_ENV_FLAG globally, but not the
rollout percent that decides the cohort. Cleaning only one leaves a later
test on a cohort it never chose, so both now reset together.
…rce uses

createDependencyPinningSource is handed effectiveProjectId (projectUUID ??
projectId), so bucketing the guard on the raw projectId let the check and the
snapshot it guards classify a multi-project request into different cohorts --
the same disagreement this PR set out to remove.

Adds coverage for the allowlist arm of the cohort. That test does not prove
which identity is read: where the two disagree, other guards answer 409 with
the identical body, so the response cannot distinguish them.
@kojiwakayama
kojiwakayama enabled auto-merge August 6, 2026 06:21
@kojiwakayama
kojiwakayama added this pull request to the merge queue Aug 6, 2026
@kwakayama kwakayama mentioned this pull request Aug 6, 2026
@kwakayama
kwakayama removed this pull request from the merge queue due to a manual request Aug 6, 2026
@kwakayama

Copy link
Copy Markdown
Contributor Author

Superseded by #3417, which consolidates this with the other queued render-path fixes plus the v0.1.1206 bump so one queue pass lands the set.

Dequeued from the merge queue to avoid this merging underneath the consolidated branch and leaving it empty or conflicting. Please do not re-queue — merge #3417 instead. Closing once that lands.

@kwakayama kwakayama closed this Aug 6, 2026
@kwakayama
kwakayama deleted the fix/dependency-pinning-cohort-409 branch August 6, 2026 06:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant